feat(CLI): Typed rendering for call output - #2179
Conversation
|
Hi @igamigo! Quick question about the changelog for this PR: there is already an entry saying “Added miden-cli call command for invoking account procedures directly from the CLI”. Should we reuse that existing entry for this PR as well, or to add a new changelog entry that explicitly mentions typed rendering for the call output? |
I think we can reuse it and add this PR link to the same entry since it's very related, but not a strong opinion so if you prefer otherwise that's fine too. |
|
Can you expand the PR description explaining the design a bit and how it relates to the issue? And/or maybe add some examples of how this renders. I know there is some context there already but the actual implementation and the scope has not been fully discussed AFAICT |
|
Hi @igamigo thanks for your comment! I updated the PR description. There's one thing I just realized, and because of it I'm turning this into a draft: I accidentally tested it against a compiler branch that isn't merged into next yet, this branch is the one that stores the high-level types inside the debug sections. My bad, I didn't check that my |
There was a problem hiding this comment.
Approve direction, but let's hold non-draft on the items below. Thanks @marijamijailovic!
Phase 1 from #2098 is covered.
Before flipping to non-draft though:
- Test coverage. See inline on
package_types/mod.rs—bool,u32,u64are priority types from #2098 but have no unit test. - Compiler dependency. See inline on
tests/cli.rs— the typed path only triggers because the test stubs the debug sections in by hand. - CHANGELOG. The check is still red and I can't drop an inline since
CHANGELOG.mdisn't in the diff. Per your thread with @igamigo, extending the existing #1943 entry sounds right, something like:
Added
miden-cli callcommand for invoking account procedures directly from the CLI (#1943), with typed argument encoding and typed result rendering when the package carries debug type info (#2179).
Follow-ups to track separately, not here:
- Phase 2
SchemaTyperendering. - Module naming:
package_typesreads ambiguous next to the manifest's ownFunctionType. Happy to be overruled.
a74dc47 to
927cc8f
Compare
|
I’ve pushed a follow-up that addresses the review comments on the typed
|
|
cc @BrianSeong99 I am tagging you here so you are aware of this one |
|
Hi @igamigo! Would it make sense to start the review while keeping this PR in draft until the compiler changes land, and then make this one “ready” once the compiler is ready as well? |
igamigo
left a comment
There was a problem hiding this comment.
Overall I'm not entirely sure the new package_debug_info should be part of miden_client. It seems like it encompasses a bunch of package/compiler-related helpers and is not really used in the miden-client crate, only the CLI. In this sense, I wonder if it should be placed either in the CLI directly or as a separate compiler crate, as it's very much independent of the codebase here.
Thanks, that’s a good point. I agree this probably live in I'm thinking about moving the whole typed encoder/decoder into This way the split would be: the compiler writes the debug sections, Let me know what do you think? And we can tag in people from the miden-vm / compiler side who can share their opinion. |
|
Hey @marijamijailovic / @Keinberger, just a heads up, if we want this on the |
|
This PR is blocked for now.
After the fix is introduce, I will:
So this PR stays in draft until then. |
5c1cb85 to
69c7a49
Compare
|
Hey @igamigo, rebased on main, miden-vm PR is open — 0xMiden/miden-vm#3276 |
The above issue is resolved. |
|
With the issue resolved are we now ready to move this PR out of draft and have it reviewed? Or were we waiting on the VM patch release? |
Thanks for bumping this up! Now that the |
|
Hi @marijamijailovic, would we be ready to move this PR out of draft or is there anything missing? |
Hi, not yet, as per comment, I'm now waiting for @bitwalker PR to land first, and then to update how |
|
@marijamijailovic 0.25.8 has landed, and the new debugger has shipped, so I think all the necessary pieces are in place for you now |
69c7a49 to
707e7cd
Compare
igamigo
left a comment
There was a problem hiding this comment.
LGTM! Holding off on approving until we have upstream dependencies merged but leaving some mostly minor comments for now.
Also, can you update the PR description? It seems to have gotten stale.
| #[error("client has not been synced yet")] | ||
| #[diagnostic( | ||
| code(cli::not_synced), | ||
| help("Run `{} sync` first.", client_binary_name().display()) | ||
| )] | ||
| NotSynced, |
| miden-client-sqlite-store = { workspace = true } | ||
| miden-debug = { optional = true, workspace = true } | ||
| miden-mast-package = { workspace = true } | ||
| miden-protocol = { workspace = true } |
There was a problem hiding this comment.
We should not need this import. AFAIK all types you are using are already being re-exported from miden-client
There was a problem hiding this comment.
Thanks, fixed this
|
|
||
| if found_without_signature { | ||
| return Err(missing_signature(procedure_name)); | ||
| } |
There was a problem hiding this comment.
Previously it seems that such procedures would be callable with u64 args and a stack dump. Is this a regression? If so we might want to add the "breaking" tag to the changelog.
There was a problem hiding this comment.
Ah, yes, that was a mistake, thanks for catching it.
I think we should always keep an option to call a procedure even when there’s no wit signature.
| /// Part of the transaction kernel's assertion message for a transaction that changes nothing. | ||
| /// | ||
| /// The assertion carries a message rather than a code the executor exposes, so this is matched as | ||
| /// text. It only decides whether an explanatory line is printed: if the kernel ever rewords it, | ||
| /// the line stops appearing and nothing else changes. | ||
| const EMPTY_TRANSACTION_ASSERTION: &str = "neither changed the account state"; |
There was a problem hiding this comment.
I think we should be able to remove this and do something like
let is_empty_transaction = matches!(
&e,
ClientError::TransactionExecutorError(
TransactionExecutorError::TransactionProgramExecutionFailed(exec_err),
) if ERR_EPILOGUE_EXECUTED_TRANSACTION_IS_EMPTY.matches_execution_error(exec_err)
);Which is still not great but at least it does not depend on substring matching against a local const.
Drop the local package_debug_info module and consume the typed encoder/decoder from miden-mast-package's debug_info::typed instead.
7ebbf1f to
988b6ff
Compare
865bcd2 to
c95500d
Compare
This PR extends the
callcommand:miden callnow reads the procedure’s types from the package manifest. It prints the signature with type names, takes each argument as a single token of its own type, and prints the result the same way.Argument conventions:
felt: 56word: 0x0100…account-id: 0xa591…asset: <AMOUNT>::<FAUCET_ID>The types come from the WIT signature in the manifest, and the encoding and decoding live in
miden-vm, in miden_mast_package::typed. The CLI only adds codecs for account-idand asset, which need protocol rules that the VM doesn’t have.A procedure exported without a WIT signature still works: its arguments are read as one field element each, and the result is printed as a stack dump.
Closes #2098